Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Better documentation of podDisruptionBudget for Elasticsearch.spec #7155

Merged
merged 6 commits into from
Oct 5, 2023
Merged

Better documentation of podDisruptionBudget for Elasticsearch.spec #7155

merged 6 commits into from
Oct 5, 2023

Conversation

vanny96
Copy link
Contributor

@vanny96 vanny96 commented Sep 6, 2023

As described in the comments at pkg/controller/elasticsearch/pdb/reconcile.go "The default PDB we setup dynamically adapts MinAvailable to the number of nodes in the cluster."

The documentation should reflect this default value, as the current one is misleading and leads to a very different behaviour.

@botelastic botelastic bot added the triage label Sep 6, 2023
@thbkrkr thbkrkr added the >docs Documentation label Sep 6, 2023
@botelastic botelastic bot removed the triage label Sep 6, 2023
@thbkrkr
Copy link
Contributor

thbkrkr commented Sep 6, 2023 •
edited
Loading

Thank you for the report. This is related to #1775 where we didn't update the documentation.

The default PodDisruptionBudget is a not exactly what you wrote.

By default, the operator sets MinAvailable to NumberOfPods - 1, so only one Pod can be disrupted.
However, in cases such as:

  • cluster not green (best-effort based on potential out-of-date information)
  • only 1 master node
  • only 1 data node

MinAvailable is set to NumberOfPods, so no Pod can be disrupted.

We also need to update the description in the values.yaml file of the eck-elasticsearch Helm chart.

cloud-on-k8s/deploy/eck-stack/charts/eck-elasticsearch/values.yaml

Line 129 in ac28d19

# By default, if not set, the operator sets a budget that selects all Elastisearch cluster pods and sets `maxUnavailable` to 1.

@vanny96
Copy link
Contributor Author

vanny96 commented Oct 2, 2023

Sorry for the late reply
Just updated the description in both the helm chart values.yaml and in the docs file. I'm not sure of the phrasing, as I was trying to keep it as short as possible while still conveying the correct information. What do you think? I wouldn't mind someone coming up with better messages!

@thbkrkr thbkrkr added the v2.10.0 label Oct 4, 2023
@thbkrkr thbkrkr requested review from pebrc and naemono October 4, 2023 07:58
@thbkrkr
Copy link
Contributor

thbkrkr commented Oct 5, 2023

buildkite test this

@thbkrkr
Copy link
Contributor

thbkrkr commented Oct 5, 2023

@elasticmachine run elasticsearch-ci/docs

@thbkrkr
Copy link
Contributor

thbkrkr commented Oct 5, 2023

I'm not sure of the phrasing, as I was trying to keep it as short as possible while still conveying the correct information. What do you think?

Your proposal is very good, we will take it as such. I just moved the description into the code and regenerated everything from that. Thanks again for the contribution.

@thbkrkr thbkrkr enabled auto-merge (squash) October 5, 2023 14:43
@thbkrkr thbkrkr merged commit 2b67b75 into elastic:main Oct 5, 2023
@rhr323 rhr323 changed the title Fixes podDisruptionBudget description for Elasticsearch.spec Better documentation of podDisruptionBudget for Elasticsearch.spec Oct 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thbkrkr thbkrkr thbkrkr approved these changes

@pebrc pebrc Awaiting requested review from pebrc

@naemono naemono Awaiting requested review from naemono

Assignees
No one assigned
Labels
>docs Documentation v2.10.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@vanny96 @thbkrkr